I have some concerns about introducing a global background MediaCleaner goroutine as the primary mechanism for file cleanup.

From a resource lifecycle perspective, it’s generally safer to follow an ownership principle: the component that creates a resource should also be responsible for cleaning it up. In the previous implementation, cleanup was explicit (e.g. via defer os.Remove(...)) and tightly coupled to the request/event lifecycle. That made ownership and timing clear.

With the current approach:

Cleanup is now time-based (e.g. “older than 30 minutes”) rather than usage-based.

The deletion logic is detached from the code that creates and consumes the file.

Lifecycle becomes implicit and harder to reason about.

Time-based cleanup is not equivalent to “no longer in use.” If any processing chain legitimately exceeds the configured threshold (slow downstream services, retries, backpressure, long-running tasks), files could be deleted while still logically in use. Even if this avoids premature deletion in some race conditions, it replaces a deterministic lifecycle with a heuristic one.

I understand the motivation behind avoiding premature cleanup, but I think we can achieve that without shifting ownership to a global janitor goroutine.

Some possible alternatives:

Have DownloadFile (or equivalent) return a cleanup func() along with the path. The caller invokes it once processing is truly complete. This keeps ownership explicit while still allowing flexible timing.

Organize temp files under a per-request/per-session directory, and remove the entire directory once the request lifecycle ends. This keeps cleanup scoped and easier to reason about.

Keep MediaCleaner only as a safety net (e.g. for orphaned files after crashes), rather than as the primary cleanup mechanism. Explicit cleanup should remain the normal path; the background cleaner should be a fallback.

In short, I think we should prefer deterministic, ownership-based cleanup over time-based sweeping. The latter can be a useful safeguard, but it shouldn’t replace explicit lifecycle management.

Thanks for the thoughtful review, @Zhaoyikaiii! These are valid concerns.

I agree that ownership-based cleanup is generally preferable. However, the challenge here is the async boundary — HandleMessage publishes the message (with file paths) to a message bus via PublishInbound, and the agent loop consumes it in a separate goroutine. There is currently no completion callback or signal from the consumer side, so the channel handler has no way to know when the agent is actually done with the file.

This means the alternatives run into the same fundamental issue:

• Cleanup func: The handler could receive one, but it doesn't know when to call it — the agent loop's completion is invisible to the handler.
• Per-request directory: Same problem — there's no signal to trigger the directory removal.
• Explicit cleanup + safety net: Without a "processing complete" signal across the async bus, there's nothing to drive explicit cleanup.

Adding such a signal (e.g., a completion callback in the message bus, or reference counting on media files) would be a more significant architectural change and is probably beyond the scope of this bug fix.

That said, the 30-minute threshold is intentionally conservative — typical message processing completes in seconds. But I understand the concern about it being heuristic rather than deterministic. Happy to discuss further or hold this PR if a broader architectural approach is preferred.

Thanks for the clarification — I agree the missing “processing complete” signal across PublishInbound/agent loop makes deterministic cleanup hard right now.

Given that, could we make the MediaCleaner behavior configurable so deployments can choose their trade-off explicitly?

Suggestions:

• Add config:
  • media_cleanup_enabled (default: true)
  • media_cleanup_max_age (default: 30m)
  • media_cleanup_interval (default: 5m)
  • (optional) media_cleanup_dir
• Log the effective config at startup (helps with debugging “why was my file deleted?”).
• Consider making the default max_age more conservative for slow/embedded setups.

This keeps the current approach as a pragmatic fix, while letting operators tune/disable it if their workloads have longer async processing times or they prefer external temp management.

Good suggestion, @Zhaoyikaiii — just pushed an update that makes MediaCleaner fully configurable via tools.media_cleanup in the config:

{
  "tools": {
    "media_cleanup": {
      "enabled": true,
      "max_age_minutes": 30,
      "interval_minutes": 5
    }
  }
}

Also supported via environment variables: PICOCLAW_MEDIA_CLEANUP_ENABLED, PICOCLAW_MEDIA_CLEANUP_MAX_AGE, PICOCLAW_MEDIA_CLEANUP_INTERVAL.

The effective config is now logged at startup for easier debugging. Operators can disable cleanup entirely or tune the thresholds for their specific deployment.

(Note: the remaining CI lint failure is a pre-existing gofmt issue in registry_test.go, unrelated to this PR.)

Perhaps you could try rebasing to resolve the CI issue; the latest code in the repository should all pass CI.

Thanks @alexhoshina — just rebased onto latest main. The pre-existing registry_test.go lint issue should be resolved now. CI should go green.

Thanks for the thorough review, @nikolasdehor!

Heads-up: since this PR, the approach has evolved. @alexhoshina asked me to integrate the cleanup directly into FileMediaStore on the refactor/channel-system branch instead of keeping it as a standalone goroutine. That's now up as #728 — TTL cleanup happens atomically under the same mutex as the ref/scope maps, so no dangling references are possible.

This PR (#620) will likely be superseded by #728 once it's merged. Your observations about first-tick cleanup and config placement are noted and still relevant there.

Closing in favor of #728, which was merged into refactor/channel-system. The TTL cleanup is now integrated directly into FileMediaStore rather than as a separate MediaCleaner, avoiding dangling ref issues.